Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP: Add ability to list images on push #4660

Closed
wants to merge 1 commit into from
Closed

WIP: Add ability to list images on push #4660

wants to merge 1 commit into from

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Dec 9, 2019 •
edited
Loading

Due to containers-storage enhancements we're now able to utilize the read-only API for different use cases. One use case is listing container images during a push, like:

> podman run -d --rm --name=registry -p 5000:5000 registry
> podman pull quay.io/crio/nix:1.1.0
> podman tag quay.io/crio/nix:1.1.0 localhost:5000/nix
> podman push --tls-verify=false localhost:5000/nix

Open a second terminal during the push operation (should take some seconds depending on the hardware) and try it out:

> podman images
REPOSITORY                    TAG         IMAGE ID       CREATED          SIZE
…

Podman is not blocking any more during a pull operation because listing container images needs only a read-only store.

Needs containers/storage#473

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 9, 2019
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: saschagrunert
To complete the pull request process, please assign vrothberg
You can assign the PR to them by writing /assign @vrothberg in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 9, 2019
@saschagrunert saschagrunert force-pushed the list-images-on-push branch 2 times, most recently from 4d542aa to 84e8326 Compare December 9, 2019 12:23
@openshift-ci-robot openshift-ci-robot added size/L and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 9, 2019
@baude
Copy link
Member

baude commented Dec 9, 2019

when you are closer to merging this, can you elaborate on the commit message?

@saschagrunert
Copy link
Member Author

when you are closer to merging this, can you elaborate on the commit message?

Yes, I'm just trying to figure out if we can do this automatically without letting libpod manually indicate that we need a "read only layer store only".

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #4675) made this pull request unmergeable. Please resolve the merge conflicts.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2019
@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #4664) made this pull request unmergeable. Please resolve the merge conflicts.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2019
@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Jan 13, 2020

@saschagrunert Still working on this one?

@saschagrunert
Copy link
Member Author

@saschagrunert Still working on this one?

Yes it's currently blocked by containers/storage#493 and containers/storage#473 where I'm currently not sure why it is failing here: #4696 🤔

@openshift-ci-robot openshift-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L labels Feb 1, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2020
@saschagrunert saschagrunert force-pushed the list-images-on-push branch 8 times, most recently from a94841b to 328a1de Compare February 11, 2020 13:36
@saschagrunert
Copy link
Member Author

Green ✔️

@mheon
Copy link
Member

mheon commented Feb 11, 2020

OK to drop the WIP?

@saschagrunert
Copy link
Member Author

OK to drop the WIP?

Unfortunately not, since we still have to

replace github.com/containers/storage => github.com/saschagrunert/storage v1.12.3-0.20200211092915-485fad532f1b

for demonstration purposes.

@mheon
Copy link
Member

mheon commented Feb 11, 2020

Ack, so we're holding on storage changes - SGTM. I'll take a look.

vrothberg
vrothberg reviewed Feb 12, 2020
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor nit. Besides that LGTM (once we merged the c/storage PR).

test/system/050-stop.bats
[ $delta_t -le 2 ] ||\
die "podman stop: took too long ($delta_t seconds; expected <= 2)"
[ $delta_t -le 3 ] ||\
die "podman stop: took too long ($delta_t seconds; expected <= 3)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes here seem unrelated and should be in a separate commits explaining why they're needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I have no idea, do you saw this in other PRs failing as well? I constantly had a higher delay in that test... 🤷‍♂️

@saschagrunert saschagrunert force-pushed the list-images-on-push branch 2 times, most recently from 02f6c68 to e067263 Compare February 12, 2020 08:57
@mheon
Copy link
Member

mheon commented Feb 12, 2020

Same, LGTM once the storage PR goes green

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 25, 2020
@openshift-ci-robot
Copy link
Collaborator

@saschagrunert: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Collaborator

@saschagrunert: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/images bb4f77a link /test images

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Mar 27, 2020

@saschagrunert Any movement on this?

@saschagrunert
Copy link
Member Author

@saschagrunert Any movement on this?

Still depends on containers/storage#473, I'll ping there.

@github-actions github-actions bot closed this Mar 31, 2021
@saschagrunert saschagrunert deleted the list-images-on-push branch March 31, 2021 07:22
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@vrothberg vrothberg vrothberg left review comments

@rhatdan rhatdan rhatdan left review comments

@giuseppe giuseppe Awaiting requested review from giuseppe

@mheon mheon Awaiting requested review from mheon

Assignees
No one assigned
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. stale-pr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@saschagrunert @openshift-ci-robot @baude @rh-atomic-bot @rhatdan @mheon @vrothberg @TomSweeneyRedHat